feat(analyzer): add phase-1 structured skill summaries - #211

Requesting changes — the phase-1 structured-skill summary is well-built and thoroughly tested, but it introduces an unhandled-crash (DoS) path on adversarial input, which matters because the scanner's whole job is to ingest untrusted skills.

Blocking — unbounded recursion → uncaught RecursionError in the core build_context path.
structured_skill.py's walkers (_extract_function_names, _extract_constraint_anchors._walk, _extract_resource_anchors._walk) recurse per nesting level with no depth bound. A crafted *.aisop.json with deeply nested functions/constraints/resources (~2k levels) raises RecursionError. _parse_bundle_path only wraps json.loads in try/except (OSError, json.JSONDecodeError) and then calls _parse_bundle_payload(...) outside that guard, so the error propagates through extract_structured_skill_context() into build_context() — a core node, not an isolated analyzer — and crashes the whole scan. (json.loads itself tolerates the nesting; the pure-Python walk is what blows the stack. Reproduced at depth≈2000.) _is_structured_skill_bundle() in multi_skill.detect_skills has the same exposure during detection.

Suggested fix: bound the nesting depth in the walkers (return/skip past a sane cap) and/or have _parse_bundle_path treat a bundle that raises during parsing as "not a valid bundle" (e.g. also catch RecursionError/ValueError around _parse_bundle_payload) so a malformed bundle fails closed to None instead of crashing the scan. A regression test with a deeply nested bundle asserting the scan completes would lock this in.

Everything else looks good — the two-message contract validation is appropriately defensive, the SSR-1 finding is correctly LOW/informational, and the build_context/detection/registry wiring and tests are solid. Happy to re-review once the recursion is bounded.

The previously reported recursion/DoS issue is resolved: all structured-metadata walkers now enforce a depth cap, parser failures are contained, and the new tests cover fail-closed behavior for functions and resources. However, this head no longer merges cleanly with current main; there are content conflicts in tests/nodes/test_build_context.py and tests/unit/test_cli.py. Please rebase or merge current main and preserve both sets of tests.

[Automated SkillSpector Review]

Re-review: still requesting changes. The prior recursion/DoS guard is resolved and conflicting tests are reconciled. However, SSR-1 is wired as an ordinary LOW finding: it changes risk scoring despite being described as informational, and the meta-analyzer can remove it entirely. Keep the summary report-visible but scoring-neutral and exempt from vulnerability filtering, with LLM and no-LLM regressions.

[Automated SkillSpector Review]

Re-review of the SSR-1 rework (head 808f476). The report-only rework does what the previous review asked: SSR-1 now flows through a dedicated structured_summaries state channel, the analyzer returns findings: [], and the summary renders in terminal/markdown/JSON/SARIF notifications while staying out of risk scoring, meta filtering, JSON issues, SARIF results, and MCP findings (verified against meta_analyzer.py, report.py, and mcp_server.py on current main).

Prior-issue resolution checklist

• Unbounded recursion / RecursionError DoS in structured_skill.py walkers (2026-06-27) — Resolved. All walkers enforce _MAX_BUNDLE_NESTING = 128 via _ensure_supported_nesting, and _parse_bundle_path catches RecursionError/ValueError so over-nested bundles fail closed to None. Regression tests with depth-140 trees exist in tests/nodes/test_build_context.py and tests/test_multi_skill.py.
• Merge conflicts in tests/nodes/test_build_context.py / tests/unit/test_cli.py (2026-06-30) — Resolved. PR is mergeable against main and both the structured-skill tests and the upstream test additions are present.
• SSR-1 scored as an ordinary LOW finding (2026-07-09) — Resolved. SSR-1 is emitted only via structured_summaries; the risk score is computed solely from findings. test_report_json_structured_summary_survives_llm_mode and the CLI --no-llm test assert risk_score == 0 / SAFE with the summary present, and the summary payload carries no severity/confidence.
• SSR-1 subject to meta filtering, could disappear in LLM mode (2026-07-09) — Resolved. meta_analyzer consumes and returns only findings/filtered_findings and never reads or writes structured_summaries; report reads summaries directly from state. Presence is asserted with use_llm=True (report node) and via a full CLI --no-llm run.

New blocker

_iter_aisop_files filters on absolute path.parts, so bundle detection silently fails when the scan root sits under a hidden or skip-listed ancestor directory. Both filter checks (part in _SKIP_DIRS over path.parts, and the hidden-dir check over path.parts[:-1]) evaluate every component of the full path, including directories above skill_dir. Scanning a skill at ~/.claude/skills/foo (the canonical installed-skill location) matches .claude and returns no bundles; an ancestor literally named venv or node_modules does the same. Reproduced against this head: a valid bundle is detected in a plain directory but returns None under .../.claude/skills/... and .../venv/.... The failure is a silent false negative in build_context() (no SSR-1 summary) and in multi_skill.detect_skills() — with --recursive, a structured-only child skill under such a path is not recognized as a skill at all, so it is never scanned. For a security scanner that is a detection gap. Fix: filter on components relative to the scan root, e.g. rel_parts = path.relative_to(skill_dir).parts and apply both checks to rel_parts (hidden-dir check on rel_parts[:-1]). Please add a regression test with the skill directory nested under a dot-directory (e.g. tmp_path / ".claude" / "skills" / "foo").

Non-blocking notes

• report() returns structured_summaries (the sanitized list) on a channel declared with an operator.add reducer, so after a graph run the final state contains each summary twice (the analyzer's raw copy plus the sanitized copy). No current consumer reads final-state structured_summaries (CLI and MCP read report_body/findings), but a future consumer would get duplicates and might pick the unsanitized copy. Consider dropping the key from report()'s return or making the channel last-value.
• _first_non_empty in structured_skill.py is a misnomer: it returns the deduplicated union of all candidate lists, not the first non-empty one. Rename (e.g. _merged_string_list) or adjust semantics.
• The one-line reformat in src/skillspector/nodes/analyzers/mcp_least_privilege.py is unrelated to this feature (harmless, likely a formatter artifact of the rebase).

[Automated SkillSpector Review]

Re-review of head 6c51fb1 — approved. The remaining blocker is resolved. _iter_aisop_files() now evaluates skip and hidden-directory filters against paths relative to the scan root, so canonical installations under ancestors such as .claude and directories named venv no longer lose structured bundle detection. Regressions cover both build_context() and recursive multi-skill discovery while retaining skip behavior for directories inside the scanned root.

The earlier recursion bounds, report-only SSR-1 channel, risk/meta-filter isolation, and merge-conflict fixes remain intact. I found no new correctness, security, schema, or test blocker in the post-review change.

[SkillSpector Review]

Re-review approved. Structured-bundle filtering is now relative to the scan root, so .claude/venv ancestors no longer hide valid bundles while actual skip-directory descendants remain excluded. Build-context and recursive-discovery regressions cover both cases. CI currently awaits repository authorization (action_required) and remains a merge gate.

Re-approved after updating to current main. The only conflict was additive in tests/unit/test_cli.py; both the structured-summary regression and main's finding-count regressions were preserved. Targeted local validation passed (143 tests, lint, format), and all required GitHub CI checks are green on head 395126d.